Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parquet_ans_processor #455

Merged
merged 9 commits into from
Jul 25, 2024
Merged

Add parquet_ans_processor #455

merged 9 commits into from
Jul 25, 2024

Conversation

yuunlimm
Copy link
Contributor

@yuunlimm yuunlimm commented Jul 11, 2024

Description

  • Migrate ans_primary_name_v2 over to BQ, which includes v1

Test Plan

Screenshot 2024-07-11 at 11 04 15 AM
Screenshot 2024-07-11 at 11 04 12 AM

@rtso rtso requested a review from a team July 12, 2024 15:32
}
}

// Parse V1 ANS write set changes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to also parse V2 ANS write set changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have to parse V2 ans wsc?

@yuunlimm yuunlimm requested a review from a team July 12, 2024 19:38
@yuunlimm yuunlimm force-pushed the yuunlimm/parquet-ans-processor branch from 67ec5f1 to 6e3a15e Compare July 16, 2024 22:18
@yuunlimm yuunlimm requested a review from a team July 16, 2024 22:19
Comment on lines +28 to +36
pub txn_version: i64,
pub write_set_change_index: i64,
pub registered_address: String,
pub token_standard: String,
pub domain: Option<String>,
pub subdomain: Option<String>,
pub token_name: Option<String>,
pub is_deleted: bool,
#[allocative(skip)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ying-w let me know if it's missing any fields

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing expiration_timestamp and subdomain_expiration_policy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rtso do you know how to parse out the expiration timestamps?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary names don't have expiration timestamp, only ans lookups do. This PR looks like it's missing moving ans_lookup_v2 to Parquet @yuunlimm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realized that this processor is still writing to the v1 ANS tables

all_current_ans_lookups,
all_ans_lookups,
all_current_ans_primary_names,
all_ans_primary_names,

We can include these in the deprecated tables and turn it off

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think ans_lookup_v2 was a part of parquet migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ying-w do we need lookups in BQ?

Copy link
Contributor

@ying-w ying-w Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realized this was for ans_primary_name_v2 not ans_lookup_v2

Yes we use both ans_lookup_v2 and current_ans_primary_name_v2 and current_ans_lookup_v2

if it's just removing ans_primary_name_v2 i think that is ok w/out moving to parquet but if going to remove current that would like the non-current to be moved to parquet

@yuunlimm yuunlimm force-pushed the yuunlimm/parquet-ans-processor branch from 6e3a15e to 10efc4b Compare July 24, 2024 20:42
#[async_trait]
impl ProcessorTrait for ParquetAnsProcessor {
fn name(&self) -> &'static str {
ProcessorName::AnsProcessor.into()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want a processor name that can differentiate parquet and regular ones? I see this method is implemented in different ways: 1) from processor name 2) hard coding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! will fix this.

@yuunlimm yuunlimm force-pushed the yuunlimm/parquet-ans-processor branch from 76a0e6b to 96228f8 Compare July 25, 2024 17:30
@yuunlimm yuunlimm merged commit 312280b into main Jul 25, 2024
7 checks passed
@yuunlimm yuunlimm deleted the yuunlimm/parquet-ans-processor branch July 25, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants